Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update option changes toast and switch their tests to jest #6174

Merged
merged 6 commits into from
Aug 23, 2023

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Aug 19, 2023

It's possible to review commit at a time (a lil messy) or all at once.

This updates option toasts to reload the window (for C#), localize them, and enable them to be tested with jest.

Jest provides the following benefits:

  1. Actively maintained, including mocking tools.
  2. VSCode extension that includes a test window where we can run/debug single tests. Previously we had to hand write custom launch.json files and couldn't run single tests.
  3. Easily able to mock the 'vscode' library for unit tests. Previously we had to always pass around the 'vscodeAdapter' abstraction in production code to allow unit tests to work. With jest, we can just use the regular 'vscode' library as normal in production code.
    a. This is essentially a requirement as we add more localization strings - in order for automatic generation of the json files to work, we must use the real 'vscode' library in-place and not the vscodeAdapter.

Commits:

  1. f80420d - Updates the option toast to reload the window for the C# case to workaround Project information is lost after language server restart with C# devkit #5882
  2. 4737540 - Localize options toasts.
  3. 70b7a37 - Add support for jest tests and migrate options tests over to jest.
  4. f4b9617 - Merge Ankita's test refactoring in.
  5. 0532fa9 - Move non-o# tests out of O# folder and add non-O# leg.

import { Options } from '../../src/shared/options';
import { registerOmnisharpOptionChanges } from '../../src/omnisharp/omnisharpOptionChanges';

import * as jestLib from '@jest/globals';
Copy link
Member Author

@dibarbet dibarbet Aug 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to globally import jest type definitions so you can just use 'describe' - however the functions will conflict with mocha ones. So for now we manually import the type definitions so we can alias them.

We also need to name it something not 'jest' as it will throw errors because jest is already imported (as an alias) by the framework

import * as vscode from 'vscode';
import { getVSCodeWithConfig, updateConfig } from '../../test/unitTests/fakes';

jestLib.describe('OmniSharpConfigChangeObserver', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are essentially the same tests, just migrated to jest.

comment: string | string[];
},
...args: (string | number | boolean)[] | Record<string, any>[]
) => {
Copy link
Member Author

@dibarbet dibarbet Aug 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - apparently this is how you have to implement an interface that defines overloads - define one function that handles every possible variation on the input. Either I'm missing something (extremely likely) or TS is just insane.

@@ -1,6 +1,7 @@
{
"recommendations": [
"dbaeumer.vscode-eslint",
"esbenp.prettier-vscode"
"esbenp.prettier-vscode",
"orta.vscode-jest"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this adds the jest test explorer - which is how you run and debug tests without manually configuring launch.json

@@ -999,4 +999,16 @@ export interface vscode {
sessionId: string;
openExternal(target: Uri): Thenable<boolean>;
};

l10n: {
Copy link
Member Author

@dibarbet dibarbet Aug 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need this vscode adapter type currently-

  1. The old mocha tests (and the production code being tested) still require it to run.
  2. The new jest tests currently require it to define the fake 'vscode' to export. We can potentially replace this fake with real mocks once everything has migrated over.

However - as we migrate more tests we can slowly remove it from being referenced in production code. I've done this partially for the option observers.

pool:
name: NetCore-Public
demands: $(demandsName)
steps:
- template: azure-pipelines/test.yml

- stage: Test_Omnisharp
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a separate leg for O# vs blue here - we actually had a few unit tests for blue, but they were running incorrectly under the O# folder. I moved them to the regular tests folder and added a leg to run them in CI as well

@dibarbet dibarbet marked this pull request as ready for review August 19, 2023 01:32
@dibarbet dibarbet requested a review from a team as a code owner August 19, 2023 01:32
Copy link
Member

@akhera99 akhera99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added CI test leg looks good to me!

@dibarbet
Copy link
Member Author

dibarbet commented Aug 23, 2023

going to merge, will take additional feedback in a followup if any.

@dibarbet dibarbet merged commit 0d2c3fe into dotnet:main Aug 23, 2023
6 checks passed
@dibarbet dibarbet deleted the jesting branch August 23, 2023 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants